Various changes to improve parsing of the "--channels" option #6
Conversation
thierer
force-pushed
the
improve_parsing_of_channels_option
branch
from
44dd34b
to
7a0e13d
Compare
|
At first I had the (now third) patch that removes the g_strsplit() checks as the first change, but reordered them to make it easier to only apply the first two patches. I just found that the wording of the commit message still reflected the situation before the other two patches so I just force-pushed an amended commit message for the third patch. |
|
There are devices which use all-number channel names. Don't assume that all devices have to start with letters just because the popular ones do. :-) Are you certain about the split related part of the PR? I was to understand that the number 2 was the maximum number of splits, which is differrent from the number of resulting "words", which again differs from the number of array items when the terminating NULL is counted. |
I'm not sure if your comment implies that you think my patch breaks ranges for numeric channel names? I don't think that's the case?
The docs for g_strsplit() state
And this program has this output So the number is the maximum of what you refer to as "words" to return (plus an additional NULL to mark the end of the vector), not the number of splits (which I assume you would expect to return 3 strings in this example, right?). |
|
Must have confused the glib split call with Python's, sorry. Will have another look later. |
Arguments to the "--channels" option are checked for the range separator (a dash) first before handling a possible rename. This makes it impossible to rename a channel to anything that includes a dash. So instead check for a rename (equals character) first and handle a possible range later. It makes no sense to rename a range of channels so report an error if there is a dash character in the part preceding the equals sign, except if a channel with this exact name exists.
Selecting a range of channels only works if the channel names are purely numeric, but not if they have a common, non-numeric prefix (eg as in "D0", "D1", ..). This patch allows to use such channels in ranges by checking for a common prefix. The syntax to for example select channels "D2" through to "D5" is "D2-5". Also adjust the manpage to include the new syntax and fix the example that actually didn't work as the fx2lafw driver uses "D0", "D1", ... as channel names.
1. !names[0] could only be true if tokens[i] is an empty string, but we already checked at the beginning of the for() loop that this is not the case. "(names[1] && names[2])" can also never be true, as g_strsplit(x, 2) returns a NULL-terminated pointer array with either one or a maximum of two non-NULL strings, so either names[1] or names[2] is always == NULL. 2. Likewise, !range[0] could only be true if names[0] is an empty string. But we already asserted that it contains at least a dash character. So both range[0] and range[1] at least contain pointers to empty strings. g_strsplit(x, 2) returns a NULL-terminated pointer array with a maximum of (and in this case, exactly) two non-NULL strings, which means range[2] is always == NULL. As a result, "!range[0] || !range[1] || range[2]" can never be true.
thierer
force-pushed
the
improve_parsing_of_channels_option
branch
from
7a0e13d
to
d0d2754
Compare
Right now, it isn't possible to rename a channel to anything that includes a dash ("-") character, as the parsing code then tries to handle the option as a channel range, which it is not. This makes it impossible to for example correctly name a usb data "D-" signal. So check if the option string contains an equals character (for a channel rename) first.
As it is, the whole "channel range" feature isn't very useful, as it only works for purely numeric channel names and most (all?) drivers seem to name their channels with a common, non-numeric prefix (like D0, D1, ...). So allow a common prefix in a channel range.
The third patch isn't strictly necessary, it only removes two checks that I consider useless but right now don't do any harm (that could change as one of the checks seems to rely on undocumented glib behaviour, see the commit message).